resetaccs!! before evaluating
#1013
Conversation
Benchmark Report for Commit 4a275a9Computer InformationBenchmark Results |
| function Base.:(==)( | ||
| acc1::PointwiseLogProbAccumulator{wlp1}, acc2::PointwiseLogProbAccumulator{wlp2} | ||
| ) where {wlp1,wlp2} | ||
| return (wlp1 == wlp2 && acc1.logps == acc2.logps) | ||
| end | ||
|
|
There was a problem hiding this comment.
Unfortunately the test that I wrote (checking that all the accs are == reset(acc) originally broke because most of them don't have an appropriate == method defined, so I had to introduce these (which mimic other changes we've made previously). I wonder if there's a better way of doing this than all the boilerplate that I introduced here though.
There was a problem hiding this comment.
I wonder if there's a better way of doing this than all the boilerplate that I introduced here though.
I suspect not, not without some sketchy metaprogramming or doing a loop over fieldnames or something else that feels dangerous.
| function _zero(acc::ValuesAsInModelAccumulator) | ||
| return ValuesAsInModelAccumulator(empty(acc.values), acc.include_colon_eq) | ||
| end |
There was a problem hiding this comment.
I preferred using _zero over Base.zero because the latter would effectively make this method public, and it doesn't seem like there's any reason to, since the public interface is split and reset.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1013 +/- ##
==========================================
+ Coverage 82.16% 82.30% +0.14%
==========================================
Files 38 38
Lines 3935 3945 +10
==========================================
+ Hits 3233 3247 +14
+ Misses 702 698 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 16827424307Details
💛 - Coveralls |
penelopeysm
force-pushed
the
py/resetaccs
branch
from
050d719 to
cc3133f
Compare
|
DynamicPPL.jl documentation for PR #1013 is available at: |
|
|
||
| # Force single-threaded execution. | ||
| DynamicPPL.evaluate_threadunsafe!!(model, varinfo) | ||
| _, varinfo = DynamicPPL.evaluate_threadunsafe!!(model, varinfo) |
| function Base.:(==)( | ||
| acc1::PointwiseLogProbAccumulator{wlp1}, acc2::PointwiseLogProbAccumulator{wlp2} | ||
| ) where {wlp1,wlp2} | ||
| return (wlp1 == wlp2 && acc1.logps == acc2.logps) | ||
| end | ||
|
|
There was a problem hiding this comment.
I wonder if there's a better way of doing this than all the boilerplate that I introduced here though.
I suspect not, not without some sketchy metaprogramming or doing a loop over fieldnames or something else that feels dangerous.
|
I'll merge but will hold off on the release |
Previously we had
resetlogp!!(vi). This set the varinfo's logp field to zero.Now we don't just have logp but also other kinds of accumulators. In general, everywhere we called
resetlogp!!(e.g. at the beginning of model evaluation), we do also want to zero out all the other accumulators.This PR introduces
DynamicPPL.reset(acc), andDynamicPPL.resetaccs!!(vi)which resets every acc.Note that
resetaccs!!is not exported. Should we? The only place in Turing that usesresetlogp!!is particle MCMC (in fact I'm unsure whether it really needs it, but that's another matter).In general pMCMC also uses
setacc!!, which is definitely unexported. We also have a bunch of fiddling around with accumulators (e.g. ValuesAsInModelAcc), the constructors for which are all unexported.